perf(phase 1): low-risk isolated fixes — leaks, bounded cache, main-actor media, micro-opts - #653
Open
mehmetefeaytas wants to merge 6 commits into
Open
perf(phase 1): low-risk isolated fixes — leaks, bounded cache, main-actor media, micro-opts#653mehmetefeaytas wants to merge 6 commits into
mehmetefeaytas wants to merge 6 commits into
Conversation
…y SMC key scan Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…8601 formatter Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…main-actor media publish, stats/battery micro-opt)
Contributor
📝 WalkthroughWalkthroughThe PR moves media playback-state mutations onto the main actor, replaces thumbnail dictionary caching with bounded ChangesMedia state coordination
Thumbnail cache management
System monitoring and audio resources
Battery observer registration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
DynamicIsland/components/Shelf/Services/ThumbnailService.swift (1)
83-86: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBase cache cost on backing pixels if 64 MB is a real memory target.
NSImage.sizeis not necessarily the underlying pixel size; representation points can differ frompixelsWide/pixelsHigh. Retina or multi-representation thumbnails can therefore be undercounted, allowing substantially more actual image memory than the configured cost limit. (developer.apple.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/components/Shelf/Services/ThumbnailService.swift` around lines 83 - 86, Update estimatedCost(of:) to calculate cache cost from the image representation’s backing pixel dimensions (pixelsWide and pixelsHigh) rather than NSImage.size, using the applicable representation for the thumbnail and preserving the 4-bytes-per-pixel estimate and minimum cost of 1.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DynamicIsland/components/Shelf/Services/ThumbnailService.swift`:
- Line 46: Update the cache-key matching in ThumbnailService’s scoped
invalidation logic to enforce a path boundary: match the exact-file prefix
"\(url.path)_" for file invalidation, or apply a directory-component boundary
predicate if callers require directory-wide invalidation. Preserve invalidation
of intended entries while excluding similarly prefixed unrelated paths.
- Around line 37-39: Update ThumbnailService’s cache bookkeeping so cacheKeys is
removed or kept synchronized with NSCache eviction; store each thumbnail’s path
on its cached wrapper object and use NSCacheDelegate.willEvictObject(_:) to
remove evicted paths, while preserving clearCache(for:) path-based invalidation.
In `@DynamicIsland/managers/BatteryActivityManager.swift`:
- Around line 322-325: Update notifyObservers(event:) to snapshot the current
observer values before entering the callback loop, then iterate over that
snapshot instead of self.observers.values. Preserve the existing main-thread
dispatch and weak-self handling while allowing callbacks to mutate observers
safely.
- Around line 40-41: Make the public observer APIs, including addObserver and
removeObserver, `@MainActor-isolated` so all accesses to observers and
nextObserverToken occur on the main actor, including the event delivery path.
Ensure callers and callback registration/removal preserve this isolation without
introducing unsynchronized registry access.
In `@DynamicIsland/managers/StatsManager.swift`:
- Around line 1078-1099: Update aggregateNetworkStats to return an optional
tuple, preserving nil when snapshots is nil instead of converting failure to
zero. In updateSystemStats, only replace previousNetworkStats when aggregation
succeeds; otherwise retain the existing cached baseline. Keep the explicit zero
fallback for the initial-baseline call sites where no prior state exists.
In `@DynamicIsland/MediaControllers/AmazonMusicController.swift`:
- Around line 302-304: Construct and publish each media state snapshot
atomically on the main actor by reading the current playback state and merging
the incoming payload inside the same update transaction. Apply this to
AmazonMusicController.swift (302-304), NowPlayingController.swift (270-272),
AppleMusicController.swift (165-168), SpotifyController.swift (199-202), and
YouTubeMusicController.swift (448-451); preserve incremental updates while
preventing stale full responses from overwriting newer fields.
---
Nitpick comments:
In `@DynamicIsland/components/Shelf/Services/ThumbnailService.swift`:
- Around line 83-86: Update estimatedCost(of:) to calculate cache cost from the
image representation’s backing pixel dimensions (pixelsWide and pixelsHigh)
rather than NSImage.size, using the applicable representation for the thumbnail
and preserving the 4-bytes-per-pixel estimate and minimum cost of 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 03066bbe-58a4-441b-814f-a62eafc3225c
📒 Files selected for processing (10)
DynamicIsland/MediaControllers/AmazonMusicController.swiftDynamicIsland/MediaControllers/AppleMusicController.swiftDynamicIsland/MediaControllers/NowPlayingController.swiftDynamicIsland/MediaControllers/SpotifyController.swiftDynamicIsland/MediaControllers/YouTube Music Controller/YouTubeMusicController.swiftDynamicIsland/components/Shelf/Services/ThumbnailService.swiftDynamicIsland/managers/BatteryActivityManager.swiftDynamicIsland/managers/StatsManager.swiftDynamicIsland/managers/SystemMediaControllers.swiftDynamicIsland/utils/CPUSensorCollector.swift
Comment on lines
+40
to
+41
| private var observers: [Int: (BatteryEvent) -> Void] = [:] | ||
| private var nextObserverToken: Int = 0 |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
rg -n 'addObserver\(|removeObserver\(byId:' --glob '*.swift'Repository: Ebullioscopic/Atoll
Length of output: 2580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "repo status:"
git status --short --untracked-files=no || true
echo
echo "top-level files:"
git ls-files | sed -n '1,80p'
echo
echo "tracetrace files named manager swift:"
git ls-files | rg 'BatteryActivityManager\.swift$|BatteryActivityManager' || true
echo
echo "try reading target Swift file:"
file="$(git ls-files | rg '((^|/)BatteryActivityManager\.swift$)' | head -n1)"
if [ -n "$file" ]; then
echo "FILE=$file"
wc -l "$file"
sed -n '1,140p' "$file"
echo "----- bottom relevant if any -----"
sed -n '280,335p' "$file"
fi
echo
echo "tracked Swift occurrences by caller/grep:"
rg -n 'addObserver\(|removeObserver\(byId:' --glob '*.swift' . || true
echo
echo "all swift occurrences in non-lfs files if available:"
rg -n 'addObserver\(|removeObserver\(byId:' . --glob '*.swift' || trueRepository: Ebullioscopic/Atoll
Length of output: 2552
Force main-thread safety for the observer registry.
addObserver and removeObserver share mutable registry state with the delivery path. If either is called from a background thread, it can race with main-thread dictionary reads/writes and token allocation, causing duplicate callbacks, dictionary corruption, or a crash. Make the public observer API @MainActor or protect all registry access with one serial synchronization primitive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DynamicIsland/managers/BatteryActivityManager.swift` around lines 40 - 41,
Make the public observer APIs, including addObserver and removeObserver,
`@MainActor-isolated` so all accesses to observers and nextObserverToken occur on
the main actor, including the event delivery path. Ensure callers and callback
registration/removal preserve this isolation without introducing unsynchronized
registry access.
Comment on lines
+302
to
+304
| await MainActor.run { [weak self] in | ||
| self?.playbackState = newPlaybackState | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Construct and publish each state snapshot in the same main-actor transaction.
These closures only publish a snapshot; its baseline was read and merged beforehand off-actor. A command/WebSocket/artwork update may modify one field on the main actor before an older full snapshot arrives and overwrites it. Build the next state from self.playbackState inside the same MainActor.run block, or main-actor-isolate the update method.
DynamicIsland/MediaControllers/AmazonMusicController.swift#L302-L304: merge the adapter payload from an actor-isolated baseline.DynamicIsland/MediaControllers/NowPlayingController.swift#L270-L272: merge the stream payload from an actor-isolated baseline.DynamicIsland/MediaControllers/AppleMusicController.swift#L165-L168: deriveupdatedStatefrom an actor-isolated baseline.DynamicIsland/MediaControllers/SpotifyController.swift#L199-L202: derive and publishresolvedStateatomically with respect to other state updates.DynamicIsland/MediaControllers/YouTube Music Controller/YouTubeMusicController.swift#L448-L451: prevent full polling/WebSocket responses from reverting incremental updates.
📍 Affects 5 files
DynamicIsland/MediaControllers/AmazonMusicController.swift#L302-L304(this comment)DynamicIsland/MediaControllers/NowPlayingController.swift#L270-L272DynamicIsland/MediaControllers/AppleMusicController.swift#L165-L168DynamicIsland/MediaControllers/SpotifyController.swift#L199-L202DynamicIsland/MediaControllers/YouTube Music Controller/YouTubeMusicController.swift#L448-L451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DynamicIsland/MediaControllers/AmazonMusicController.swift` around lines 302
- 304, Construct and publish each media state snapshot atomically on the main
actor by reading the current playback state and merging the incoming payload
inside the same update transaction. Apply this to AmazonMusicController.swift
(302-304), NowPlayingController.swift (270-272), AppleMusicController.swift
(165-168), SpotifyController.swift (199-202), and YouTubeMusicController.swift
(448-451); preserve incremental updates while preventing stale full responses
from overwriting newer fields.
- ThumbnailService: reconcile `cacheKeys` against the NSCache once it exceeds the count limit so the tracking set stays bounded; match the "<path>_" boundary in clearCache(for:) so invalidating /tmp/a no longer evicts /tmp/ab's thumbnails. - BatteryActivityManager: snapshot observers (Array(values)) before invoking, so a callback that adds/removes observers can't trigger a mutation-during-iteration trap. - StatsManager: preserve the network baseline when getifaddrs fails (nil snapshot) instead of collapsing to (0,0), which faked a speed dip and wiped previousNetworkStats. - SystemMediaControllers: drop the volume/mute element caches — they were read/populated from the public API on arbitrary threads while refreshPropertyElements() cleared them on callbackQueue (data race). Recompute on access (cheap probe). HAL listener-leak fix retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
|
Thanks @coderabbitai — addressed in 3193e1d. ✅ Applied
↩️ Deferred (with rationale)
Tests: added standalone logic tests for the path-boundary match, observer-token add/remove (no id shift + snapshot-during-mutation), and network-baseline-on-failure — all pass. App builds clean ( |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The main-actor merge/publication race is reasonably deferred from this low-risk phase. Would you like me to open a GitHub follow-up issue for it? 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
DynamicIsland/managers/SystemMediaControllers.swift (2)
223-231: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not discard the active listeners before the replacement device is validated.
If
resolveDefaultDevice()transiently fails and returns0, or the new device cannot be probed, this removes the working listeners and installs none. Preserve the existing registration until the new device/listener set is successfully resolved, or retain the previous device on resolution failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/managers/SystemMediaControllers.swift` around lines 223 - 231, Update installVolumeListeners(for:) to validate resolveElement and the complete replacement listener setup before calling removeVolumeListeners(). If the new device resolves to 0 or cannot be probed, preserve the existing volumeElement and listeners; only replace the current registration after the new device/listener set is successfully resolved.
253-265: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetain registrations when listener removal fails.
After a non-
noErrremoval, the HAL listener may still be active, butremoveAll()drops the device/address/block needed to retry removal. Repeated route changes can therefore leak listeners and produce duplicate callbacks. Keep failed registrations for retry, or abort the listener-set swap until cleanup succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/managers/SystemMediaControllers.swift` around lines 253 - 265, The removeVolumeListeners method currently discards all registrations even when AudioObjectRemovePropertyListenerBlock fails. Preserve registrations whose removal returns a non-noErr status so they can be retried, while removing successfully deregistered entries; ensure volumeListenerRegistrations is only updated with the failed registrations rather than cleared unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@DynamicIsland/managers/SystemMediaControllers.swift`:
- Around line 223-231: Update installVolumeListeners(for:) to validate
resolveElement and the complete replacement listener setup before calling
removeVolumeListeners(). If the new device resolves to 0 or cannot be probed,
preserve the existing volumeElement and listeners; only replace the current
registration after the new device/listener set is successfully resolved.
- Around line 253-265: The removeVolumeListeners method currently discards all
registrations even when AudioObjectRemovePropertyListenerBlock fails. Preserve
registrations whose removal returns a non-noErr status so they can be retried,
while removing successfully deregistered entries; ensure
volumeListenerRegistrations is only updated with the failed registrations rather
than cleared unconditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 03f25ccf-1fb5-415d-bcfe-6c5a604d6ab4
📒 Files selected for processing (4)
DynamicIsland/components/Shelf/Services/ThumbnailService.swiftDynamicIsland/managers/BatteryActivityManager.swiftDynamicIsland/managers/StatsManager.swiftDynamicIsland/managers/SystemMediaControllers.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- DynamicIsland/managers/BatteryActivityManager.swift
- DynamicIsland/components/Shelf/Services/ThumbnailService.swift
- DynamicIsland/managers/StatsManager.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 of a staged performance pass (low-risk, isolated, no behavior change). Each area was implemented and build-verified independently, then merged.
Changes
SystemMediaControllers.swift):SystemVolumeControlleradded property listeners on every default-device change but never removed the old ones → leaked listeners + duplicate notifications on each headphone swap. Now tracks registrations and removes before re-installing; also memoizes the volume/mute element probes (invalidated on device change).ThumbnailService.swift): unbounded[String: NSImage]→NSCache(countLimit 200, totalCostLimit 64 MB, per-image cost). Path-scoped clearing preserved via a tracked key set; drop/dedup logic untouched.MediaControllers/*):@Published playbackStatewas written from background stream/websocket/notification contexts ("Publishing changes from background threads"). All state writes moved onto the main actor (state still computed off-main, then a single atomic assignment). Also reuse astaticISO8601DateFormatterinstead of allocating per stream line.BatteryActivityManager.swift,StatsManager.swift,utils/CPUSensorCollector.swift): fixed the index-based observer id (shifting ids on removal) → monotonic token dict; periodicpsrefresh now respects the 2 s throttle (was bypassed viaforce: true); collapsed two per-tickgetifaddrswalks into one; SMC primary-temperature scan is now early-exit + caches the working key.Expected gains (estimates — verify with Instruments)
Verification
xcodebuild … CODE_SIGNING_ALLOWED=NO build→ BUILD SUCCEEDED, no new warnings.First of a 4-phase plan; subsequent phases (background-load/energy, main-thread offload, architecture) follow as separate PRs.
Summary by CodeRabbit
Bug Fixes
Performance Improvements